Auto bind component data-test-* attributes #27

marcoow · 2016-12-02T10:55:06Z

This is the follow-up PR for #26.

Instead of automatically inserting data-test-* attributes for all components with the component's internal name as a value we're now simply auto-binding all data-test-* attributes that are present on a component.

marcoow · 2016-12-02T11:21:30Z

@Turbo87: the preprocessor that strips data-test-* attributes from components' JS and the assignments in the templates might be sth. for you to look into in Jan.

Turbo87 · 2017-01-02T10:58:17Z

config/ember-try.js

@@ -1,6 +1,5 @@
 /*jshint node:true*/
 module.exports = {
-  useVersionCompatibility: true,


@marcoow was this an intentional change? as discussed earlier today we should most likely test all minor Ember releases since we're using an Handlebars AST transform here and the AST is not public/stable.

I guess this was just a result of running ember init after updating Ember CLI.

Turbo87 · 2017-01-02T10:59:14Z

.travis.yml

+  # Usually, it's ok to finish the test scenario without reverting
+  #  to the addon's original dependency state, skipping "cleanup".
+  - ember try:one $EMBER_TRY_SCENARIO test --skip-cleanup
+  - npm run mocha


this is failing because there is no mocha script defined. what's the reason for using mocha if all you do is check JSHint compliance?

Hm, don't remember to be honest 😊

Turbo87 · 2017-01-02T10:59:28Z

package.json

    "ember-resolver": "^2.0.3",
-    "loader.js": "^4.0.1",
+    "ember-welcome-page": "^1.0.3",


this is most likely not needed here

Turbo87 · 2017-01-02T11:00:12Z

package.json

-    "ember-cli-babel": "^5.1.5",
-    "lodash": "^4.0.0"
+    "ember-cli-babel": "^5.1.7",
+     "lodash": "^4.0.0"


extra leading space here

Turbo87 · 2017-01-02T14:18:48Z

rebased this on top of #29 and #31

Turbo87 · 2017-01-10T10:22:08Z

@marcoow @pangratz I've updated this PR with a few changes:

I've extracted a bind-data-test-attributes utility which is called by the initializer
The initializer and actually everything else inside addon and app is now stripped from the build if we strip test selectors
Added tests for the new utility function and the initializer

Turbo87 · 2017-01-10T10:42:28Z

It looks like the automatic binding of component attributes is failing on Ember 1.13 and 2.0. Do we care about compatibility with those releases? Should I investigate further if we might be able to work around this?

marcoow · 2017-01-10T10:51:02Z

addon/utils/bind-data-test-attributes.js

+    }
+  }
+
+  set(component, 'attributeBindings', attributeBindings);


I generally prefer component.set('attributeBindings', attributeBindings).

marcoow · 2017-01-10T10:54:42Z

@Turbo87: I'd like to keep support for 1.13 if possible and certainly 2.0 unless that's really hard to do for some reason.

... that reopens the Component class and automatically binds all data-test-* attributes

pangratz

nice ❤️

marcoow · 2017-01-10T11:46:40Z

addon/utils/bind-data-test-attributes.js

+    }
+  }
+
+  component.set('attributeBindings', attributeBindings);


I guess this doesn't work though if the original attributeBindings is a CP?

that is true, but I'm not sure if attributeBindings being a CP is supported by Ember at all. otherwise there would be no need for us to apply the bindDataTestAttributes() function before the super constructor.

Yeah good point. Could you check this though?

that is true, but I'm not sure if attributeBindings being a CP is supported by Ember at all.

I'm now checking if attributeBindings is a computed property in the bindDataTestAttributes() function and if so, I skip the automatic binding and display a warning message.

…er super()

…property

Turbo87 · 2017-01-10T12:30:49Z

@marcoow I've figured out the problems with the early Ember versions: when used like {{some-component data-test-foo="bar"}} the data-test-foo property is assigned in the Ember.Object constructor in Ember 1.13 and 2.0 and is not available before the super() call where we call bindDataTestAttributes(). I've moved the binding call behind the super() call and it now seems to work in all Ember releases that we test against.

marcoow · 2017-01-11T08:35:56Z

addon/utils/bind-data-test-attributes.js

+
+    return Ember.warn(message, false, {
+      id: 'ember-test-selectors.computed-attribute-bindings',
+    });


If we could somehow make it work correctly in this case as well that would be awesome of course. In theory it should be possible to replace the CP with a wrapper one that we create of course but I'm not sure it's worth the added complexity.

I'm not sure it's worth the extra complexity to be honest. I've never seen anyone use a CP for attributeBindings and I have quite a hard time coming up with use cases where that would be a good idea. I'd suggest keeping the warning for now, and if many users report that they are seeing this warning then we can still think about implementing it.

marcoow · 2017-01-11T08:37:09Z

tests/acceptance/bind-data-test-attributes-in-components-test.js

+    assert.equal(find('.test6').find('div[data-non-test]').length, 0, 'data-non-test does not exists');
+  });
+
+}


We should have a test for the case where the component defines attributeBindings (both as a string as well as an array) itself and that asserts that the data-test-* attributes get merged with these correctly.

@marcoow we test that in the unit tests for the bindDataTestAttributes() function. Can attributeBindings be a string? Unfortunately it's not documented anywhere but I've only found instances where it is used as an array.

Hm, just assumed both might actually be supported. If we have a unit test for this already that should be fine.

Since attributeBindings is a concatenated property, the string will end up in an array eventually. This only works for:

attributeBindings: "myAttribute"

which will become

attributeBindings: ["myAttribute"]

and will not work for:

attributeBindings: "myAttribute myOtherAttribute"

which would result in attributeBindings: ["myAttribute myOtherAttribute"]

http://emberjs.com/api/classes/Ember.View.html#toc_html-attributes only uses arrays, so I'm assuming passing a string is not officially supported.

Since it's not official API, I would tend to agree with @Turbo87 and not support this tbh.

I've implemented a quick array wrapper now, see f48e6ad

marcoow assigned marcoow and Turbo87 Dec 2, 2016

Turbo87 reviewed Jan 2, 2017

View reviewed changes

Turbo87 mentioned this pull request Jan 2, 2017

Update dependencies #29

Merged

Turbo87 force-pushed the auto-bind-component-attrs branch from 0cc827d to a22acce Compare January 2, 2017 14:18

Turbo87 added 2 commits January 9, 2017 18:18

Add linting for "addon" and "app" folders

50257c3

Adjust addon name to "ember-test-selectors"

af385c6

Turbo87 force-pushed the auto-bind-component-attrs branch from a22acce to 6de34db Compare January 10, 2017 10:19

Turbo87 requested a review from pangratz January 10, 2017 10:19

Turbo87 force-pushed the auto-bind-component-attrs branch from 6de34db to f2bc2da Compare January 10, 2017 10:40

marcoow commented Jan 10, 2017

View reviewed changes

Turbo87 added 3 commits January 10, 2017 12:21

Add "bind-data-test-attributes" utility function

0d31bc0

Add "ember-test-selectors" initializer

3e69cc5

... that reopens the Component class and automatically binds all data-test-* attributes

Strip "app" and "addon" folders from the build

151aad0

Turbo87 force-pushed the auto-bind-component-attrs branch from f2bc2da to 151aad0 Compare January 10, 2017 11:21

pangratz approved these changes Jan 10, 2017

View reviewed changes

marcoow commented Jan 10, 2017

View reviewed changes

Turbo87 added 2 commits January 10, 2017 13:00

initializers/ember-test-selectors: Apply bindDataTestAttributes() aft…

b75c9f4

…er super()

utils/bind-data-test-attributes: Skip and print warning for computed …

7868eda

…property

marcoow commented Jan 11, 2017

View reviewed changes

utils/bind-data-test-attributes: Convert string to array if necessary

f48e6ad

marcoow merged commit d93d364 into master Jan 11, 2017

marcoow deleted the auto-bind-component-attrs branch January 11, 2017 10:04

Turbo87 added the enhancement label Aug 30, 2017

Provide feedback

Saved searches

Use saved searches to filter your results more quickly

Auto bind component data-test-* attributes #27

Auto bind component data-test-* attributes #27

marcoow commented Dec 2, 2016

marcoow commented Dec 2, 2016

Turbo87 Jan 2, 2017

marcoow Jan 2, 2017

Turbo87 Jan 2, 2017

marcoow Jan 2, 2017

Turbo87 Jan 2, 2017

Turbo87 Jan 2, 2017

marcoow Jan 2, 2017

Turbo87 commented Jan 2, 2017

Turbo87 commented Jan 10, 2017

Turbo87 commented Jan 10, 2017

marcoow Jan 10, 2017

Turbo87 Jan 10, 2017

marcoow commented Jan 10, 2017

pangratz left a comment

marcoow Jan 10, 2017

Turbo87 Jan 10, 2017

marcoow Jan 10, 2017

Turbo87 Jan 10, 2017

Turbo87 commented Jan 10, 2017

marcoow Jan 11, 2017

Turbo87 Jan 11, 2017

marcoow Jan 11, 2017

Turbo87 Jan 11, 2017

marcoow Jan 11, 2017

pangratz Jan 11, 2017

Turbo87 Jan 11, 2017

pangratz Jan 11, 2017

Turbo87 Jan 11, 2017

Auto bind component data-test-* attributes #27

Auto bind component data-test-* attributes #27

Conversation

marcoow commented Dec 2, 2016

marcoow commented Dec 2, 2016

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Turbo87 commented Jan 2, 2017

Turbo87 commented Jan 10, 2017

Turbo87 commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

marcoow commented Jan 10, 2017

pangratz left a comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Turbo87 commented Jan 10, 2017

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment

Choose a reason for hiding this comment